[Fizz] Implement New Context #21255
Merged
[Fizz] Implement New Context #21255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This implements a reverse linked list tree containing the previous contexts.
This algorithm pops the contexts back to a shared ancestor on the way down the stack and then pushes new contexts in reverse order up the stack.
This is primarily intended to be used to support renderToString with a separate build than the main one. This allows them to be nested.
facebook-github-bot
added
React Core Team
sebmarkbage
commented
Apr 13, 2021
| // for that. Therefore this algorithm is recursive. | ||
| // 1) First we pop which ever snapshot tree was deepest. Popping old contexts as we go. | ||
| // 2) Then we find the nearest common ancestor from there. Popping old contexts as we go. | ||
| // 3) Then we reapply new contexts on the way back up the stack. |
There was a problem hiding this comment.
@sophiebits You might enjoy this algorithm.
I also considered only storing the "value" and not depth/previousValue. That can be used to create a mode that always pushes all contexts - unless the new context is a strict superset. That could make the whole thing a bit lighter and still be fast when it only unsuspends in strict parent->child order.
But I think the pure common ancestor approach is probably worth it.
sebmarkbage
commented
Apr 13, 2021
| @@ -1148,6 +1319,16 @@ function performWork(request: Request): void { | |||
| fatalError(request, error); | |||
| } finally { | |||
| ReactCurrentDispatcher.current = prevDispatcher; | |||
| if (prevDispatcher === Dispatcher) { | |||
There was a problem hiding this comment.
This case is meant to handle something like:
function Foo() {
renderToString(<Bar />);
}
renderToString(<Foo />);In a follow up.
sebmarkbage
commented
Apr 13, 2021
| @@ -48,6 +48,10 @@ import hasOwnProperty from 'shared/hasOwnProperty'; | |||
| import sanitizeURL from '../shared/sanitizeURL'; | |||
| import isArray from 'shared/isArray'; | |||
|
|
|||
| // Used to distinguish these contexts from ones used in other renderers. | |||
| // E.g. this can be used to distinguish legacy renderers from this modern one. | |||
| export const isPrimaryRenderer = true; | |||
There was a problem hiding this comment.
I plan on building renderToString and Fizz in separate renderers so this will be used to deal with this case:
function Foo() {
renderToString(<Bar />);
}
renderToWritable(<Foo />);
|
Comparing: dbadfa2...5acef5f Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gaearon
approved these changes
Apr 14, 2021
|
|
||
| export function popProvider<T>(context: ReactContext<T>): void { | ||
| const prevSnapshot = currentActiveSnapshot; | ||
| invariant( |
There was a problem hiding this comment.
It follows the principle that if it's getting checked by runtime type checks here anyway, it likely isn't a perf cost (other than the extra code). The prevSnapshot.context access after does that anyway.
| } else if (next === null) { | ||
| popAllPrevious(prev); | ||
| } else if (prev.depth === next.depth) { | ||
| popToNearestCommonAncestor(prev, next); |
There was a problem hiding this comment.
The naming of this and the next one confused me because they don't only pop, but also push the missing ones.
There was a problem hiding this comment.
Yea I thought that when I was naming them. Not sure what to call it. Maybe ...AndThenPush.
| // We must still be deeper. | ||
| popNextToCommonLevel(prev, parentNext); | ||
| } | ||
| pushNode(next); |
There was a problem hiding this comment.
Why is this needed? Same question re: initial pop in the previous method.
There was a problem hiding this comment.
In the case one is deeper than the other there won't be the same amount of pop/push calls. If the previous is deeper, we need to pop more times than we push. If the next is deeper, we need to push more than we pop.
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Apr 20, 2021
Summary: This sync includes the following changes: - **[f7cdc8936](facebook/react@f7cdc8936 )**: Also turn off enableSyncDefaultUpdates in RN test renderer ([#21293](facebook/react#21293)) //<Ricky>// - **[4c9eb2af1](facebook/react@4c9eb2af1 )**: Add dynamic flags to React Native ([#21291](facebook/react#21291)) //<Ricky>// - **[9eddfbf5a](facebook/react@9eddfbf5a )**: [Fizz] Two More Fixes ([#21288](facebook/react#21288)) //<Sebastian Markbåge>// - **[11b07597e](facebook/react@11b07597e )**: Fix classes ([#21283](facebook/react#21283)) //<Sebastian Markbåge>// - **[96d00b9bb](facebook/react@96d00b9bb )**: [Fizz] Random Fixes ([#21277](facebook/react#21277)) //<Sebastian Markbåge>// - **[81ef53953](facebook/react@81ef53953 )**: Always insert a dummy node with an ID into fallbacks ([#21272](facebook/react#21272)) //<Sebastian Markbåge>// - **[a4a940d7a](facebook/react@a4a940d7a )**: [Fizz] Add unsupported Portal/Scope components ([#21261](facebook/react#21261)) //<Sebastian Markbåge>// - **[f4d7a0f1e](facebook/react@f4d7a0f1e )**: Implement useOpaqueIdentifier ([#21260](facebook/react#21260)) //<Sebastian Markbåge>// - **[dde875dfb](facebook/react@dde875dfb )**: [Fizz] Implement Hooks ([#21257](facebook/react#21257)) //<Sebastian Markbåge>// - **[a597c2f5d](facebook/react@a597c2f5d )**: [Fizz] Fix reentrancy bug ([#21270](facebook/react#21270)) //<Sebastian Markbåge>// - **[15e779d92](facebook/react@15e779d92 )**: Reconciler should inject its own version into DevTools hook ([#21268](facebook/react#21268)) //<Brian Vaughn>// - **[4f76a28c9](facebook/react@4f76a28c9 )**: [Fizz] Implement New Context ([#21255](facebook/react#21255)) //<Sebastian Markbåge>// - **[82ef450e0](facebook/react@82ef450e0 )**: remove obsolete SharedArrayBuffer ESLint config ([#21259](facebook/react#21259)) //<Henry Q. Dineen>// - **[dbadfa2c3](facebook/react@dbadfa2c3 )**: [Fizz] Classes Follow Up ([#21253](facebook/react#21253)) //<Sebastian Markbåge>// - **[686b635b7](facebook/react@686b635b7 )**: Prevent reading canonical property of null ([#21242](facebook/react#21242)) //<Joshua Gross>// - **[bb88ce95a](facebook/react@bb88ce95a )**: Bugfix: Don't rely on `finishedLanes` for passive effects ([#21233](facebook/react#21233)) //<Andrew Clark>// - **[343710c92](facebook/react@343710c92 )**: [Fizz] Fragments and Iterable support ([#21228](facebook/react#21228)) //<Sebastian Markbåge>// - **[933880b45](facebook/react@933880b45 )**: Make time-slicing opt-in ([#21072](facebook/react#21072)) //<Ricky>// - **[b0407b55f](facebook/react@b0407b55f )**: Support more empty types ([#21225](facebook/react#21225)) //<Sebastian Markbåge>// - **[39713716a](facebook/react@39713716a )**: Merge isObject branches ([#21226](facebook/react#21226)) //<Sebastian Markbåge>// - **[8a4a59c72](facebook/react@8a4a59c72 )**: Remove textarea special case from child fiber ([#21222](facebook/react#21222)) //<Sebastian Markbåge>// - **[dc108b0f5](facebook/react@dc108b0f5 )**: Track which fibers scheduled the current render work ([#15658](facebook/react#15658)) //<Brian Vaughn>// - **[6ea749170](facebook/react@6ea749170 )**: Fix typo in comment ([#21214](facebook/react#21214)) //<inokawa>// - **[b38ac13f9](facebook/react@b38ac13f9 )**: DevTools: Add post-commit hook ([#21183](facebook/react#21183)) //<Brian Vaughn>// - **[b943aeba8](facebook/react@b943aeba8 )**: Fix: Passive effect updates are never sync ([#21215](facebook/react#21215)) //<Andrew Clark>// - **[d389c54d1](facebook/react@d389c54d1 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21211](facebook/react#21211)) //<Brian Vaughn>// - **[c486dc1a4](facebook/react@c486dc1a4 )**: Remove unnecessary processUpdateQueue ([#21199](facebook/react#21199)) //<Sebastian Markbåge>// - **[cf45a623a](facebook/react@cf45a623a )**: [Fizz] Implement Classes ([#21200](facebook/react#21200)) //<Sebastian Markbåge>// - **[75c616554](facebook/react@75c616554 )**: Include actual type of `Profiler#id` on type mismatch ([#20306](facebook/react#20306)) //<Sebastian Silbermann>// - **[1214b302e](facebook/react@1214b302e )**: test: Fix "couldn't locate all inline snapshots" ([#21205](facebook/react#21205)) //<Sebastian Silbermann>// - **[1a02d2792](facebook/react@1a02d2792 )**: style: delete unused isHost check ([#21203](facebook/react#21203)) //<wangao>// - **[782f689ca](facebook/react@782f689ca )**: Don't double invoke getDerivedStateFromProps for module pattern ([#21193](facebook/react#21193)) //<Sebastian Markbåge>// - **[e90c76a65](facebook/react@e90c76a65 )**: Revert "Offscreen: Use JS stack to track hidden/unhidden subtree state" ([#21194](facebook/react#21194)) //<Brian Vaughn>// - **[1f8583de8](facebook/react@1f8583de8 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21192](facebook/react#21192)) //<Brian Vaughn>// - **[ad6e6ec7b](facebook/react@ad6e6ec7b )**: [Fizz] Prepare Recursive Loop for More Types ([#21186](facebook/react#21186)) //<Sebastian Markbåge>// - **[172e89b4b](facebook/react@172e89b4b )**: Reland Remove redundant initial of isArray ([#21188](facebook/react#21188)) //<Sebastian Markbåge>// - **[7c1ba2b57](facebook/react@7c1ba2b57 )**: Proposed new Suspense layout effect semantics ([#21079](facebook/react#21079)) //<Brian Vaughn>// - **[316aa3686](facebook/react@316aa3686 )**: [Scheduler] Fix de-opt caused by out-of-bounds access ([#21147](facebook/react#21147)) //<Andrey Marchenko>// - **[b4f119cdf](facebook/react@b4f119cdf )**: Revert "Remove redundant initial of isArray ([#21163](facebook/react#21163))" //<Sebastian Markbage>// - **[c03197063](facebook/react@c03197063 )**: Revert "apply prettier ([#21165](facebook/react#21165))" //<Sebastian Markbage>// - **[94fd1214d](facebook/react@94fd1214d )**: apply prettier ([#21165](facebook/react#21165)) //<Behnam Mohammadi>// - **[b130a0f5c](facebook/react@b130a0f5c )**: Remove redundant initial of isArray ([#21163](facebook/react#21163)) //<Behnam Mohammadi>// - **[2c9fef32d](facebook/react@2c9fef32d )**: Remove redundant initial of hasOwnProperty ([#21134](facebook/react#21134)) //<Behnam Mohammadi>// - **[1cf9978d8](facebook/react@1cf9978d8 )**: Don't pass internals to callbacks ([#21161](facebook/react#21161)) //<Sebastian Markbåge>// - **[b9e4c10e9](facebook/react@b9e4c10e9 )**: [Fizz] Implement all the DOM attributes and special cases ([#21153](facebook/react#21153)) //<Sebastian Markbåge>// - **[f8ef4ff57](facebook/react@f8ef4ff57 )**: Flush discrete passive effects before paint ([#21150](facebook/react#21150)) //<Andrew Clark>// - **[b48b38af6](facebook/react@b48b38af6 )**: Support nesting of startTransition and flushSync (alt) ([#21149](facebook/react#21149)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c9aab1c...f7cdc89 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D27740113 fbshipit-source-id: 6e27204d78e3e16ed205170006cb97c0d6bfa957
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
* Add NewContext module This implements a reverse linked list tree containing the previous contexts. * Implement recursive algorithm This algorithm pops the contexts back to a shared ancestor on the way down the stack and then pushes new contexts in reverse order up the stack. * Move isPrimaryRenderer to ServerFormatConfig This is primarily intended to be used to support renderToString with a separate build than the main one. This allows them to be nested. * Wire up more element type matchers * Wire up Context Provider type * Wire up Context Consumer * Test * Implement reader in class * Update error codez
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fizz is a bit different than either Fiber or the partial renderer in terms of scenarios and tradeoffs. Unlike the partial renderer, we don't just always resume where we previously left off. We can suspend in different sibling trees. Unlike Fiber, we never have to restart with new props and we don't otherwise need the tree we've already past.
We could use a model similar to partial renderer's use of threads, where each context has N number of concurrent threads and a thread is allocated for each suspended task. But that would require a large and growing array for each Context which probably doesn't end up paying for itself.
We could use a Map of contexts associated with each provider. This requires a lot of cloning at the point of each provider. Effectively what legacy context does.
We could also store a loop up list of all parent providers that we backtrack any time we read a Context. That comes at the cost of reading being slower.
The general tradeoffs that we want to favor is making reading cheap so that mostly global dependency injection things are very cheap. We also generally favor rendering at full perf once something is unsuspended. Even though there are many possible suspended points, if you're preload a lot of data then you hopefully can just render straight through large sections without suspending at all.
The approach that I actually went with is similar to the Fiber approach in that we update the
_currentValuefield on each context and then reading is as cheap as just reading that value.I also store a linked list of the stack of context providers that we're currently in. This gets heap allocated each time we visit a provider. If we need to spawn a new task, this node is what gets stored on the Task.
When we resume work on a task, we switch context by popping and pushing the delta between the most recently active context and the task's context. I.e. we restore where we were.
The cost of this happens at the point when we switch between tasks. It's cheap (or free) when we're resuming a single task over and over. Such as when the first few root components suspend or when you've reach the last suspended subtree which is the final one blocking completion. It's slower when you're switching back and forth between two deep and far removed siblings but at that point you're not close to reaching completion anyway.
(This could use some more tests of various suspending and resuming scenarios that are specific to Fizz and isn't covered by existing tests. The coverage isn't great yet. But didn't want to spend too much time on it before I unblock other work on top.)